Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Allow classNamePrefixSeparator in image.jsx (fixes #478) #479

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Fix: Allow classNamePrefixSeparator in image.jsx (fixes #478) #479

merged 2 commits into from
Dec 8, 2023

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Dec 5, 2023 •
edited
Loading

fixes #478

Fix

  • Add option to override default className separator __ in image.jsx by specifying classNamePrefixSeparator = '__item-'

@oliverfoster oliverfoster added the bug Something isn't working label Dec 5, 2023
@oliverfoster oliverfoster self-assigned this Dec 5, 2023
@oliverfoster oliverfoster mentioned this pull request Dec 5, 2023
Merged
cahirodoherty-learningpool
cahirodoherty-learningpool approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

kirsty-hames
kirsty-hames requested changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@kirsty-hames kirsty-hames left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classNamePrefixSeparator is applied to both classNamePrefixes and attributionClassNamePrefixes, however the image attribution doesn't require a prefix separator.
Looking at the current naming in Adapt, regardless if the attribution is for a component image or component item image, we just specify the component and component type, e.g. .component__attribution .accordion__attribution. Vanilla attribution styles target these selectors too.

I'd suggest we apply classNamePrefixSeparator to classNamePrefixes only. Would you like me to make the amend @oliverfoster whilst I'm working on adaptlearning/adapt-contrib-hotgraphic#290 ?

@oliverfoster
Copy link
Member Author

Yes please. Take it away. 👍

kirsty-hames
kirsty-hames approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works as expected thanks.

@kirsty-hames kirsty-hames mentioned this pull request Dec 6, 2023
Merged
joe-allen-89
joe-allen-89 approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@joe-allen-89 joe-allen-89 merged commit bf18404 into master Dec 8, 2023
@joe-allen-89 joe-allen-89 deleted the issue/478 branch December 8, 2023 16:16
github-actions bot pushed a commit that referenced this pull request Dec 8, 2023
@semantic-release-bot
Chore(release): 6.43.2
4db5616 
## [6.43.2](v6.43.1...v6.43.2) (2023-12-08)

### Fix

* Allow classNamePrefixSeparator in image.jsx (fixes #478) (#479) ([bf18404](bf18404)), closes [#478](#478) [#479](#479) [#478](#478)
@oliverfoster
Copy link
Member Author

🎉 This PR is included in version 6.43.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-allen-89 joe-allen-89 joe-allen-89 approved these changes

@cahirodoherty-learningpool cahirodoherty-learningpool cahirodoherty-learningpool approved these changes

@kirsty-hames kirsty-hames kirsty-hames approved these changes

Assignees

@oliverfoster oliverfoster

Labels
bug Something isn't working released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use of image.jsx when rendering items has bad classNamePrefixes
4 participants
@oliverfoster @kirsty-hames @cahirodoherty-learningpool @joe-allen-89